permission: add --allow-env flag for environment variable access control#62827
permission: add --allow-env flag for environment variable access control#62827nabeel378 wants to merge 12 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
nabeel378
force-pushed
the
feat/permission-allow-env-flag
branch
from
9c74582 to
f3544f8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62827 +/- ##
==========================================
- Coverage 89.64% 89.62% -0.02%
==========================================
Files 707 709 +2
Lines 219506 219570 +64
Branches 42087 42098 +11
==========================================
+ Hits 196782 196799 +17
- Misses 14625 14660 +35
- Partials 8099 8111 +12
🚀 New features to boost your workflow:
|
|
|
||
| > Stability: 1.1 - Active development | ||
|
|
||
| When using the [Permission Model][], access to environment variables is |
There was a problem hiding this comment.
This paragraph and the console block below it describe a deny-by-default model that the code does not implement
./out/Release/node --permission '--allow-fs-read=*' -p 'process.env.HOME'
/Users/thisalihassanThere was a problem hiding this comment.
Good catch! You're right, env vars are unrestricted by default with --permission. They only get restricted when --allow-env is explicitly passed. Updated both cli.md and process.md with the correct description and working examples.
| ``` | ||
|
|
||
| ```console | ||
| $ node --permission index.js |
There was a problem hiding this comment.
As written, this output is unreachable
./out/Release/node --permission test.js
/Users/thisalihassan| The `process.env` property returns an object containing the user environment. | ||
| See environ(7). | ||
|
|
||
| When the [Permission Model][] is enabled, access to environment variables is |
There was a problem hiding this comment.
Same issue as cli.md
|
I don't think this PR as it stands closes #62424, the issue asked for a deny-by-default model with throw-on-read, matching Deno and consistent with the rest of the permission model which doesn't defend against the threat the issue was explicitly motivated by The PR description is no longer accurate according to the current opt-in model |
I missed that. Switching to deny-by-default to stay consistent with the other permission flags and update desc proper. |
RafaelGSS
requested review from
RafaelGSS
and removed request for
thisalihassan
nabeel378
force-pushed
the
feat/permission-allow-env-flag
branch
2 times, most recently
from
6a95f6e to
33a1126
Compare
Signed-off-by: nabeel378 <[email protected]>
Signed-off-by: nabeel378 <[email protected]>
Signed-off-by: nabeel378 <[email protected]>
…r environment variables Signed-off-by: nabeel378 <[email protected]>
Signed-off-by: nabeel378 <[email protected]>
Signed-off-by: nabeel378 <[email protected]>
The documentation incorrectly described env var restriction as deny-by-default when --permission is used. In reality, env vars are unrestricted by default and only become restricted when --allow-env is explicitly passed. Update cli.md and process.md to accurately reflect this behavior. Signed-off-by: nabeel378 <[email protected]>
Signed-off-by: nabeel378 <[email protected]>
nabeel378
force-pushed
the
feat/permission-allow-env-flag
branch
from
73d59fb to
4cdd6ec
Compare
|
@RafaelGSS please review. |
4 participants
Adds --allow-env permission flag to control access to environment variables when the permission model is enabled (--permission).
Supported usage:
When --permission is enabled without --allow-env, reading a restricted variable silently returns undefined, writing or deleting throws ERR_ACCESS_DENIED, and enumerating (Object.keys(process.env)) returns only allowed variables.
Fixes: #62424